Skip to content

Add some testcases and missing ASF headers for new scheduler #5243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
May 26, 2022
Merged

Add some testcases and missing ASF headers for new scheduler #5243

merged 9 commits into from
May 26, 2022

Conversation

jiangpengcheng
Copy link
Contributor

Add some testcases&missing ASF headers

Description

Related issue and scope

  • I opened an issue to propose and discuss this change (#????)

My changes affect the following components

  • API
  • Controller
  • Message Bus (e.g., Kafka)
  • Loadbalancer
  • Scheduler
  • Invoker
  • Intrinsic actions (e.g., sequences, conductors)
  • Data stores (e.g., CouchDB)
  • Tests
  • Deployment
  • CLI
  • General tooling
  • Documentation

Types of changes

  • Bug fix (generally a non-breaking change which closes an issue).
  • Enhancement or new feature (adds new functionality).
  • Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

  • I signed an Apache CLA.
  • I reviewed the style guides and followed the recommendations (Travis CI will check :).
  • I added tests to cover my changes.
  • My changes require further changes to the documentation.
  • I updated the documentation where necessary.

@jiangpengcheng jiangpengcheng changed the title Add some testcases for new scheduler and missing ASF headers Add some testcases and missing ASF headers for new scheduler May 23, 2022
style95
style95 approved these changes May 23, 2022
View reviewed changes
Copy link
Member

@style95 style95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@codecov-commenter
Copy link

codecov-commenter commented May 23, 2022 •
edited
Loading

Codecov Report

Merging #5243 (f24c226) into master (edc484b) will increase coverage by 4.56%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5243      +/-   ##
==========================================
+ Coverage   70.63%   75.20%   +4.56%     
==========================================
  Files         238      238              
  Lines       13996    13996              
  Branches      561      561              
==========================================
+ Hits         9886    10525     +639     
+ Misses       4110     3471     -639
Impacted Files Coverage Δ
.../openwhisk/core/loadBalancer/FPCPoolBalancer.scala 32.95% <ø> (+32.95%) ⬆️
...core/scheduler/queue/SchedulingDecisionMaker.scala 98.78% <ø> (+98.78%) ⬆️
...core/database/cosmosdb/RxObservableImplicits.scala 0.00% <0.00%> (-100.00%) ⬇️
...ore/database/cosmosdb/cache/CacheInvalidator.scala 0.00% <0.00%> (-100.00%) ⬇️
...e/database/cosmosdb/cache/ChangeFeedConsumer.scala 0.00% <0.00%> (-100.00%) ⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala 0.00% <0.00%> (-95.85%) ⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala 0.00% <0.00%> (-93.90%) ⬇️
...tabase/cosmosdb/cache/CacheInvalidatorConfig.scala 0.00% <0.00%> (-92.31%) ⬇️
...enwhisk/connector/kafka/KamonMetricsReporter.scala 0.00% <0.00%> (-83.34%) ⬇️
...e/database/cosmosdb/cache/KafkaEventProducer.scala 0.00% <0.00%> (-78.58%) ⬇️
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edc484b...f24c226. Read the comment docs.

@jiangpengcheng jiangpengcheng force-pushed the add_more_tests branch 2 times, most recently from f0e2019 to b5e9c91 Compare May 24, 2022 05:09
@jiangpengcheng
Copy link
Contributor Author

I think this should fix some unstable tests in new scheduler

style95
style95 reviewed May 25, 2022
View reviewed changes
tests/src/test/scala/org/apache/openwhisk/core/scheduler/queue/test/ContainerCounterTests.scala Outdated
val watcher = system.actorOf(WatcherService.props(mockEtcdClient))

val ns = NamespaceContainerCount(namespace, mockEtcdClient, watcher)
Thread.sleep(1000)
Copy link
Member

@style95 style95 May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope we avoid using this kind of waiting if possible.
Can we replace this with something like a reactive one?
It might cause some non-deterministic results of CI tests.

For example, we might periodically check the expected status during the given time limit with some interval.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaced with retry, but there are many Thread.sleep in tests files, we may need an another pr to replace them

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree.
I just wanted not to add more Thread.sleep.

@jiangpengcheng jiangpengcheng merged commit c90e8cc into apache:master May 26, 2022
JesseStutler pushed a commit to JesseStutler/openwhisk that referenced this pull request Jul 13, 2022
@jiangpengcheng @JesseStutler
Add some testcases and missing ASF headers for new scheduler (apache#…
19dd3db 
…5243)

* Add some testcases and missing ASF headers for new scheduler

* Reset stream after each test

* Wait more time

* Update msg start time to ensure it's not stale

* Make MemoryQueueFlowTests stable

* Ignore warmUp msg immediately

* Use retry to replace sleep

* Add missing config

* Fix configuration error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@style95 style95 style95 approved these changes

@ningyougang ningyougang ningyougang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jiangpengcheng @codecov-commenter @style95 @ningyougang